Introduce a configuration value for migration_path
#4190
Conversation
forkata
force-pushed
the
forkata/configurable-migration-path
branch
from
1cbc7dd to
74b276c
Compare
|
Awesome! This is exactly what I was thinking! Thank you @forkata! |
There was a problem hiding this comment.
@forkata thank you for adding this new preference 🎉
I left a suggestion for a small improvement, let me know if it works for you
Since Rails 6 multiple database support is a native feature, which allows users to configure separate read/write databases. This change allows users to configure the path to the migrations folder from the default "db/migrate" if they are using the multiple database support and have specified a different database for the Solidus engine. This will ensure the check that all migrations are installed can look at a user configurable folder. One change required to make the folder path configurable is to run the migration check after 'load_config_initializers' instead of before. This will allow the value of `migration_path` configuration to be set in an initializer. Co-authored-by: Adam Mueller <adam@super.gd> Co-authored-by: Alex Blackie <alex@super.gd> Co-authored-by: Benjamin Willems <benjamin@super.gd> Co-authored-by: Jared Norman <jared@super.gd>
forkata
force-pushed
the
forkata/configurable-migration-path
branch
from
74b276c to
9eaacc6
Compare
|
Interesting. @kennyadsl pointed out that the change to when the hook runs was added by Clarke in a commit that was trying to get the configuration stuff set up before initializers ran. My guess is that the migration check got lumped into that change not realizing it didn't really matter (unlike the rest of the hooks in that commit, which you definitely do want to run before your initializers.) |
There is a small downside to not checking migrations before running initializers and that is in the case where someone is accessing models from their initializers. I am not sure how common that use case is, so I am not too concerned about degrading the user experience with this change. |
|
@forkata We were discussing that in the Core meeting. What exact situation could someone get into where this would be a problem? |
I don't think there is a case where this would be a problem, it's more that the messaging would not be as clear as it would be prior to this change. The situation I am thinking of is you have not run a particular migration to create a table, but you are trying to reference the corresponding model from an initializer. |
Description
Since Rails 6 multiple database support is a native feature, which
allows users to configure separate read/write databases. This change
allows users to configure the path to the migrations folder from the
default "db/migrate" if they are using the multiple database support and
have specified a different database for the Solidus engine. This will
ensure the check that all migrations are installed can look at a user
configurable folder.
One change required to make the folder path configurable is to run the
migration check after 'load_config_initializers' instead of before. This
will allow the value of
migration_pathconfiguration to be set in aninitializer.
Ref #4182
Checklist:
I have attached screenshots to this PR for visual changes (if needed)